Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pitchbend direction fix lp1770745 #1906

Merged
merged 8 commits into from
Jan 4, 2019
Merged

Pitchbend direction fix lp1770745 #1906

merged 8 commits into from
Jan 4, 2019

Conversation

daschuer
Copy link
Member

@daschuer daschuer commented Nov 18, 2018

This fixes some pitchbend issues including https://bugs.launchpad.net/mixxx/+bug/1770745
See commit messages.

It also adds a test.

@daschuer daschuer added this to the 2.1.6 milestone Nov 18, 2018
@daschuer daschuer changed the base branch from master to 2.1 November 18, 2018 00:28
@daschuer
Copy link
Member Author

Before the patch we had:
Up increase, Abrupt "->" = faster (default)
Up increase, Smooth "->" = faster
Down increase, Abrupt "->" = faster
Down increase, Smooth "->" = slower (fixed with this patch)

I have tested this and with the parallel waveforms, the odd thing is that "faster" moves the waveform faster to the left, this means "<-" relative to the second deck waveform.
@ronso0 is that a usability issue?

@daschuer
Copy link
Member Author

Appveyor fails unrelated.

@ronso0
Copy link
Member

ronso0 commented Nov 22, 2018

that's a consequence of the opposite directions of the play position on the overview and the scrolling waveform moving underneath the static playposition. Neither the Play icon nor any seek or nudge icons are intuitive if they are related to the scrolling waveform. Since those buttons are located in sections of a deck they're closer to the overview, so I think the nudge icons are more intuitive as they are now than if we'd mirror them.

Maybe some kind of right-pointing arrow overlay/icon on or near the waveform play position marker would emphasize it's not the waveform underneath but actually the (static) play position line that's moving. Would that reduce the confusion?

@daschuer
Copy link
Member Author

Right. Since I have never noticed the issue before and we have no user complaining about this. So I think we can stick with the current situation.

@uklotzde
Copy link
Contributor

uklotzde commented Dec 8, 2018

Does this introduce conflicts with #1924?

Conflicts:
	src/engine/ratecontrol.cpp
	src/engine/ratecontrol.h
@uklotzde
Copy link
Contributor

I haven't checked every conditional branch, but so far the code looks good. It is at least more reasonable than before.

@daschuer
Copy link
Member Author

Does it mean we can merge this?

@daschuer daschuer modified the milestones: 2.1.6, 2.1.7 Jan 1, 2019
@uklotzde
Copy link
Contributor

uklotzde commented Jan 4, 2019

Tested successfully. LGTM.

@uklotzde uklotzde merged commit 4b3a36c into mixxxdj:2.1 Jan 4, 2019
@daschuer
Copy link
Member Author

daschuer commented Jan 4, 2019

Thank you :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants